fix: add master referenced id/uri audio playlists. Add playlists to hls media groups #1124
Conversation
brandonocasey
force-pushed
the
fix/hls-dash-mg-ids
branch
from
06c7bfc to
1a109dd
Compare
Codecov Report
@@ Coverage Diff @@
## main #1124 +/- ##
==========================================
+ Coverage 86.15% 86.19% +0.04%
==========================================
Files 39 39
Lines 9254 9281 +27
Branches 2115 2126 +11
==========================================
+ Hits 7973 8000 +27
Misses 1281 1281
Continue to review full report at Codecov.
|
brandonocasey
force-pushed
the
fix/hls-dash-mg-ids
branch
from
1a109dd to
caf93d6
Compare
brandonocasey
force-pushed
the
fix/hls-dash-mg-ids
branch
from
20fe047 to
a771af2
Compare
src/manifest.js
Outdated
| properties.playlists.forEach(function(p, i) { | ||
| const id = createPlaylistID(i, groupId); | ||
|
|
||
| p.uri = p.uri || id; |
There was a problem hiding this comment.
is p.uri expected to be the groupId (what used to be called phonyUri)?
Should uri continue to be set to groupId?
There was a problem hiding this comment.
Now that we can have more than 1 playlist in a group the groupId alone would conflict
There was a problem hiding this comment.
Wonder how big of an issue this is, since, it's potentially breaking.
There was a problem hiding this comment.
I guess we could keep playlist at index 0 as groupId, and use all others as id, then deprecate using it like this. Ideally we would do away with array/uri/id object thing we have right now.
brandonocasey
force-pushed
the
fix/hls-dash-mg-ids
branch
from
b59673d to
a771af2
Compare
| if (!properties.playlists || !properties.playlists.length) { | ||
| properties.playlists = [Object.assign({}, properties)]; | ||
| } |
There was a problem hiding this comment.
This may impact this check:
http-streaming/src/media-groups.js
Line 471 in 552b012
Which looks for the existence of playlists.
There was a problem hiding this comment.
I think this might be okay since it is looking for dash sourceType specifically here and those will always have playlists. It does change our notions around a bit though.
There was a problem hiding this comment.
If DASH always has playlists in this case, I wonder why I included that case at all back when.
There was a problem hiding this comment.
maybe it didn't at the time this was added
There was a problem hiding this comment.
It might be worth us putting a TODO on that other line just so we don't forget to look into it.
| if (!properties.playlists || !properties.playlists.length) { | ||
| properties.playlists = [Object.assign({}, properties)]; | ||
| } |
There was a problem hiding this comment.
It might be worth us putting a TODO on that other line just so we don't forget to look into it.
Fixes issues with rendition switching on audio only hls media.